Stop using loader functions, part 1 #1306

jessepinho · 2024-06-14T21:47:17Z

This PR goes through a bunch of routes where we're using react-router loaders, and replaces loader functionality with ZQuery. Note that this means some assumptions have changed about when data is available. react-router loaders wait to render their route components until data has loaded, which means your component can assume the data is already present. Since we're now effectively loading data from useEffect() (by way of ZQuery), we can't assume that data is present just because a component is rendered. So you'll see some instances of code that accounts for undefined where we didn't previously have to.

One of the benefits of this change is that route changes are instant. There's no lag time between when you click on a link to a route, and when the route changes, like there was before when route changes were blocked by loaders.

Before (note the delayed route changes after I click)

Screen.Recording.2024-06-17.at.3.45.06.PM.mov

After (note the instant route changes after I click)

Screen.Recording.2024-06-17.at.3.43.55.PM.mov

In this PR

Remove the balances loader and create a balances slice with a balancesByAccount ZQuery object.
Remove the Send form's loader and create a transferableBalancesResponses ZQuery object in the send state.
Create a shared Zustand slice and added a stakingTokenMetadata ZQuery object to it, to be used by the send form and the IBC UIs.
Remove the staking page's loader; in its place, use state.shared.stakingTokenMetadata, and combine the accountSwitcherFilter and unstakedTokensByAccount into a single ZQuery object, since they both depend on the same data.
Remove the transactions' page's loader; in its place, convert its slice to use a ZQuery object.

Future PRs

Convert the SwapLoader and IbcLoader to ZQuery. Those look a bit involved and this PR is already touching a lot of files, so I haven't done those yet.
Convert more of the staking slice to ZQuery. That would be a bit more involved as well, and doesn't involve getting rid of loaders, so I figured I'd punt on that for now.

Relates to #1243

changeset-bot · 2024-06-14T21:47:20Z

🦋 Changeset detected

Latest commit: 8601886

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@penumbra-zone/zquery	Patch
minifront	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

TalDerei

this is really great work!

TalDerei · 2024-06-18T04:59:51Z

apps/minifront/src/components/dashboard/assets-table/index.tsx


 export default function AssetsTable() {
-  const balancesByAccount = useLoaderData() as BalancesByAccount[];
+  const balancesByAccount = useBalancesByAccount();


we were previously using abortLoader to sanity-check that certain preconditions were met before proceeding with a data-fetching operation. If any of the checks failed, an exception was thrown, which aborted the loading process. Where is the equivalent of this happening in zquery land?

it should be unnecessary - the loader check was only present because the loaders were suppressing the check done at page layout

I've actually moved abortLoader to root-router.tsx.

@turbocrime are you saying I can remove them, then?

TalDerei · 2024-06-18T05:02:13Z

apps/minifront/src/abort-loader.ts

@@ -32,8 +32,12 @@ const retry = async (fn: () => boolean, ms = 500, rate = Math.max(ms / 10, 50))
 * timeout. This is a temporary solution until loaders properly await Prax
 * connection.
 */
-export const abortLoader = async (): Promise<void> => {
+export const abortLoader = async (): Promise<null> => {


Is this the trick, ie. returning null, that enables instantaneous route changes?

Ah, no — loaders are required to return something rather than void. The instantaneous route changes are due to the fact that we no longer have loaders that are awaiting any slow requests.

TalDerei · 2024-06-18T05:37:33Z

apps/minifront/src/components/root-router.tsx


-export const rootRouter = createHashRouter([
+export const rootRouter: Router = createHashRouter([


can we add some comments somewhere on the high-level similarities and differences between react-router loaders and zquery?

From what I can tell, both support:

async data fetching

state access to access fetched data in components

In terms of their differences, this is where some more clarification would be helpful. Zquery anchors on the idea of global state management, where state can be updated and shared across an application, right? React-router on the other hand fetches data specifically for a single route, and rather than being stored globally, it's only accessible by a specific component. I guess I'm wondering where component-level data fetching, a staple of the latter case, would generally be more preferred?

Yeah, hm, I'm realizing I should write up an ADR for all of this.

The idea is that we want to consolidate our state into one state machine, rather than having it split among several (Zustand, react-router, React Query, etc.).

I would assume that component-level data fetching as provided by React Query is for simpler applications that don't use a global data store like Zustand. It's also useful for server-side applications that load data and render it before sending HTML down to the browser — unlike what we're doing with our entirely client-side rendering.

TalDerei · 2024-06-18T05:44:57Z

apps/minifront/src/state/staking/index.ts

+export const { stakingTokensAndFilter, useStakingTokensAndFilter } = createZQuery({
+  name: 'stakingTokensAndFilter',
+  fetch: async () => {
+    const balancesByAccount = await getBalancesByAccount();


random question out of curiosity, can you somehow chain zquery requests? for instance, instead of calling getBalancesByAccount(), something like

// Access the state of balancesByAccount const balances = useStore.getState().balances.balancesByAccount;

Hm... That'd be an interesting use case. You could theoretically call useStore.getState().balances.balancesByAccount.revalidate(), but at the moment, revalidate() is a synchronous function that returns void, and kicks off the request in the background, updating the state when the request completes.

We could consider adding some sort of "dependency" logic to ZQuery — i.e., when one ZQuery object updates its state, it re-triggers a fetch of another ZQuery object. 🤔 Do you have a use case in mind?

don't have specific use case in mind, was just curious if this was possible.

TalDerei · 2024-06-18T05:49:01Z

good to merge barring the resolution of conflicts ~

TalDerei · 2024-06-18T06:08:34Z

apps/minifront/src/state/index.ts

@@ -18,9 +20,11 @@ import { createIbcInSlice, IbcInSlice } from './ibc-in';
 enableMapSet();

 export interface AllSlices {


can we add a comment on what slices generally are and what they represent in the context of the SliceCreator<SliceInterface> custom type?

Added a general comment here, but to be honest, I'm not super familiar with the typings and with why SliceCreator exists the way it does 😬 (that was before my time)

TalDerei · 2024-06-19T18:57:19Z

@jessepinho I still had a few questions I left as comments I was hoping you could answer, selfishly for my own benefit?

jessepinho

whoops, forgot to submit my comments

jessepinho · 2024-06-15T00:28:13Z

apps/minifront/package.json

@@ -33,6 +33,7 @@
    "@penumbra-zone/ui": "workspace:*",
    "@penumbra-zone/zquery": "workspace:*",
    "@radix-ui/react-icons": "^1.3.0",
+    "@remix-run/router": "^1.16.1",


See my comment in root-router

jessepinho · 2024-06-15T00:28:35Z

apps/minifront/src/components/root-router.tsx


-export const rootRouter = createHashRouter([
+export const rootRouter: Router = createHashRouter([


Adding : Router fixes a longtime TypeScript error we were getting in this file that for some reason didn't break the build.

jessepinho · 2024-06-17T21:50:58Z

apps/minifront/src/components/dashboard/assets-table/index.tsx

-export const AssetsLoader: LoaderFunction = async (): Promise<BalancesByAccount[]> => {
-  await abortLoader();
-  return await getBalancesByAccount();
-};


Note that I've deleted the loaders for a bunch of routes, but I've replaced their loaders in root-router.tsx with abortLoader. So, the abortLoader functionality is still there; it's just not wrapped in a separate loader colocated with the component.

jessepinho · 2024-06-17T22:20:03Z

apps/minifront/src/components/staking/account/delegation-value-view/index.tsx

-     * A `ValueView` representing the address's balance of staking (UM) tokens.
-     * Used to show the user how many tokens they have available to delegate.
-     */
-    unstakedTokens?: ValueView;


This prop was only used to pass it to <StakingActions /> below, so I've removed it now that unstakedTokens can be accessed via the stakingTokensAndFilter ZQuery object.

jessepinho · 2024-06-17T22:24:21Z

...inifront/src/components/staking/account/delegation-value-view/staking-actions/index.test.tsx

Had to add some mocks to this file because the <StakingActions /> component no longer gets all its data via args, but instead also uses some ZQuery hooks that thus need to be mocked.

jessepinho · 2024-06-19T17:17:44Z

apps/minifront/src/components/dashboard/assets-table/index.tsx


 export default function AssetsTable() {
-  const balancesByAccount = useLoaderData() as BalancesByAccount[];
+  const balancesByAccount = useBalancesByAccount();


I've actually moved abortLoader to root-router.tsx.

@turbocrime are you saying I can remove them, then?

jessepinho · 2024-06-19T17:18:23Z

apps/minifront/src/abort-loader.ts

@@ -32,8 +32,12 @@ const retry = async (fn: () => boolean, ms = 500, rate = Math.max(ms / 10, 50))
 * timeout. This is a temporary solution until loaders properly await Prax
 * connection.
 */
-export const abortLoader = async (): Promise<void> => {
+export const abortLoader = async (): Promise<null> => {


Ah, no — loaders are required to return something rather than void. The instantaneous route changes are due to the fact that we no longer have loaders that are awaiting any slow requests.

jessepinho · 2024-06-19T17:29:44Z

apps/minifront/src/components/root-router.tsx


-export const rootRouter = createHashRouter([
+export const rootRouter: Router = createHashRouter([


Yeah, hm, I'm realizing I should write up an ADR for all of this.

The idea is that we want to consolidate our state into one state machine, rather than having it split among several (Zustand, react-router, React Query, etc.).

I would assume that component-level data fetching as provided by React Query is for simpler applications that don't use a global data store like Zustand. It's also useful for server-side applications that load data and render it before sending HTML down to the browser — unlike what we're doing with our entirely client-side rendering.

jessepinho · 2024-06-19T17:32:32Z

apps/minifront/src/state/staking/index.ts

+export const { stakingTokensAndFilter, useStakingTokensAndFilter } = createZQuery({
+  name: 'stakingTokensAndFilter',
+  fetch: async () => {
+    const balancesByAccount = await getBalancesByAccount();


Hm... That'd be an interesting use case. You could theoretically call useStore.getState().balances.balancesByAccount.revalidate(), but at the moment, revalidate() is a synchronous function that returns void, and kicks off the request in the background, updating the state when the request completes.

We could consider adding some sort of "dependency" logic to ZQuery — i.e., when one ZQuery object updates its state, it re-triggers a fetch of another ZQuery object. 🤔 Do you have a use case in mind?

jessepinho · 2024-06-19T18:02:40Z

apps/minifront/src/state/index.ts

@@ -18,9 +20,11 @@ import { createIbcInSlice, IbcInSlice } from './ibc-in';
 enableMapSet();

 export interface AllSlices {


Added a general comment here, but to be honest, I'm not super familiar with the typings and with why SliceCreator exists the way it does 😬 (that was before my time)

jessepinho force-pushed the jessepinho/stop-using-loader-functions-web-1243 branch 3 times, most recently from 2a70cc1 to fca7672 Compare June 17, 2024 21:37

jessepinho changed the title ~~Stop using loader functions~~ Stop using loader functions, part 1 Jun 17, 2024

jessepinho force-pushed the jessepinho/stop-using-loader-functions-web-1243 branch from fca7672 to 2edb69f Compare June 17, 2024 21:57

jessepinho marked this pull request as ready for review June 17, 2024 22:51

jessepinho requested a review from a team June 17, 2024 22:51

TalDerei approved these changes Jun 18, 2024

View reviewed changes

TalDerei reviewed Jun 18, 2024

View reviewed changes

jessepinho added 14 commits June 19, 2024 10:39

Fix typing of getBalances

415e2c8

Fix TypeScript complaint

44952d2

Allow abortLoader to be used in root router

bb8e290

Use ZQuery for balances

033323c

Update transactions slice to use ZQuery

a46a4bc

Use ZQuery for the Send form

8e1f732

Separate out IBC in slice files for weird import reasons

7830835

Fix little TypeScript issue

fd2b9c8

Convert transaction details to ZQuery

e54075b

Move staking loader data to ZQuery

8cef12d

Fix typings issues

5f6c8b3

Remove unnecessary assignment

c21846a

Changeset

e00cc1d

Remove unused imports

ca1b276

jessepinho force-pushed the jessepinho/stop-using-loader-functions-web-1243 branch from e69f8f1 to ca1b276 Compare June 19, 2024 17:54

jessepinho added 2 commits June 19, 2024 11:02

Document AllSlices

8b5c5fc

Remove unused import

8601886

jessepinho merged commit 733d62a into main Jun 19, 2024
6 checks passed

jessepinho deleted the jessepinho/stop-using-loader-functions-web-1243 branch June 19, 2024 18:10

jessepinho commented Jun 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using loader functions, part 1 #1306

Stop using loader functions, part 1 #1306

jessepinho commented Jun 14, 2024 •

edited

Loading

changeset-bot bot commented Jun 14, 2024 •

edited

Loading

TalDerei left a comment

TalDerei Jun 18, 2024 •

edited

Loading

turbocrime Jun 18, 2024

jessepinho Jun 19, 2024

TalDerei Jun 18, 2024

jessepinho Jun 19, 2024

TalDerei Jun 18, 2024 •

edited

Loading

jessepinho Jun 19, 2024

TalDerei Jun 18, 2024

jessepinho Jun 19, 2024

TalDerei Jun 21, 2024

TalDerei commented Jun 18, 2024

TalDerei Jun 18, 2024 •

edited

Loading

jessepinho Jun 19, 2024

TalDerei commented Jun 19, 2024 •

edited

Loading

jessepinho left a comment

jessepinho Jun 15, 2024

jessepinho Jun 15, 2024

jessepinho Jun 17, 2024

jessepinho Jun 17, 2024

jessepinho Jun 17, 2024

jessepinho Jun 19, 2024

jessepinho Jun 19, 2024

jessepinho Jun 19, 2024

jessepinho Jun 19, 2024

jessepinho Jun 19, 2024


		export const rootRouter = createHashRouter([
		export const rootRouter: Router = createHashRouter([

		@@ -18,9 +20,11 @@ import { createIbcInSlice, IbcInSlice } from './ibc-in';
		enableMapSet();

		export interface AllSlices {

Stop using loader functions, part 1 #1306

Stop using loader functions, part 1 #1306

Conversation

jessepinho commented Jun 14, 2024 • edited Loading

Before (note the delayed route changes after I click)

After (note the instant route changes after I click)

In this PR

Future PRs

changeset-bot bot commented Jun 14, 2024 • edited Loading

🦋 Changeset detected

TalDerei left a comment

Choose a reason for hiding this comment

TalDerei Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei commented Jun 18, 2024

TalDerei Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei commented Jun 19, 2024 • edited Loading

jessepinho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessepinho commented Jun 14, 2024 •

edited

Loading

changeset-bot bot commented Jun 14, 2024 •

edited

Loading

TalDerei Jun 18, 2024 •

edited

Loading

TalDerei Jun 18, 2024 •

edited

Loading

TalDerei Jun 18, 2024 •

edited

Loading

TalDerei commented Jun 19, 2024 •

edited

Loading